The function name is a little misleading because readers may think it is same as contents_->estimated_buffered_value_bytes().

rename to total_buffered_bytes()

row_group_writer_->num_rows() > 0 can only happen when the current row group writer is in the buffered mode. Usually users calling WriteTable will never use buffered mode so this approach seems not working in the majority of cases.

Instead, can we gather this information from all written row groups (if available)?

@wgtmac If user use the static WriteTable function, the arrow FileWriter is always recreated and we can not gather the old written row groups.

If user use the internal WriteTable function, we can get avg_row_bytes by last row_group_writer_ or gathering all previous row group writers.

I still think this estimation does not help because in most cases WriteTable will not be used in the buffered mode. See my suggestion in the below comment.

I still think this estimation does not help because in most cases WriteTable will not be used in the buffered mode. See my suggestion in the below comment.

The chunk_size could be 0 if the configured max_row_group_bytes is less than avg_row_size, do we need a double check here?

We need to clamp the chunk size between 1 and max_row_group_bytes/avg_row_size.

ditto.

This order looks more natural :)

Perhaps rename to written_row_group_compressed_bytes_ to be more clear? Or written_compressed_bytes_ if previous one is too long.

We need to clamp the chunk size between 1 and max_row_group_bytes/avg_row_size.

Will it cause infinite loop at this line if batch_size is always 0?

It would cause infinite loop only when the max_row_group_bytes/avg_row_size is 0, is it OK to return Invalid status in WriteXxx() at this case?

        if (batch_size == 0 && row_group_writer_->num_rows() == 0) {
          return Status::Invalid(
              "Configured max_row_group_bytes is too small to hold a single row");
        }

We cannot accept infinite loop so perhaps we have to set the minimum batch size to 1 in this case?

Set the minimum batch size to 1 is not reasonable, when the buffered_bytes > max_row_group_bytes we still set the batch size as 1, then it will continually append one row to the active row group and never create a new one. Returning an invalid status might be more intuitive.

Shouldn't we check row group size after writing each batch? If a large per row size leads to batch size equal to 1, we just end up with checking row group size after writing every row.

We does not check row group size after write batch, current write logic is like:

1. check rows and bytes to determine current batch_size
2. if batch_size > 0, write these rows to current row group, it's guaranteed not exceeds the row group limits
3. if batch_size = 0 and still has rows to write, new a row group
4. next loop to 1

In this way we don't need check size after written, it's guaranteed in step 1; and we'll not leave an possible empty row group in the final batch, it guaranteed in step 3.

How do you achieve the step 4 above next loop to 1? This looks a bit complicated to me. How about the logic below.

For each loop iteration:

1. Check the row group threshold (both rows and bytes) and append a new row group if needed.
2. Compute a new batch_size based on different conditions and set its minimum to 1 so we don't get empty batch.
3. Write the batch as before.

EstimatedBufferedValueBytes does not account for compression and may therefore wildly overestimate the final compressed size?

Are we sure we want to account for contents not serialized into a page yet?

This encoding size is a reference before the first page written, and its impact diminishes as more pages are written.

I'm not sure that makes it useful in any way, though.

In many common cases, the compression ratio is close to 3:1. So I used something like total_compressed_bytes + total_compressed_bytes_written + EstimatedBufferedValueBytes / (codec_type != NONE ? 3 : 1) as an empirical value in the past.
`

Do we need this logic here? we can do it in EstimatedBufferedValueBytes::Contents::EstimatedBufferedValueBytes(), and rename it like EstimatedCompressedBufferedBytes().

@pitrou 3 also will not cause huge over-estimation as long as there are many pages. I think the only difference is:
1 will produce row group that over the max_row_group_bytes while 3 produce smaller one.

The main difference is that 3 is more complicated and more fragile. So I would rather we do 1.

If we choose 1, we can change to estimate batch size based on avg row size and written row numbers to avoid ignoring too many buffered bytes like:

    while (offset < batch.num_rows()) {
      auto avg_row_size = EstimateCompressedBytesPerRow();
      int64_t max_rows =
          avg_row_size
              ? std::min(max_row_group_length,
                         // Ensure batch_size is at least 1 to avoid infinite loops.
                         std::max(1L, static_cast<int64_t>(max_row_group_bytes /
                                                           avg_row_size.value())))
              : max_row_group_length;
      if (row_group_writer_->num_rows() >= max_rows) {
        // Current row group is full, start a new one.
        RETURN_NOT_OK(NewBufferedRowGroup());
      }
      int64_t batch_size =
          std::min(max_rows - row_group_writer_->num_rows(), batch.num_rows() - offset);
      RETURN_NOT_OK(WriteBatch(offset, batch_size));
      offset += batch_size;
    }

The last concern is that the max_row_group_bytes could not take effect before the first page is written, is it acceptable?

I agree that in most common cases, buffered values contribute to only a small fraction of the total row group size. There is a caveat in this approach: dict entries are buffered and thus not counted in this case. If we have many columns, we may underestimate a lot because dictionary encoding is enabled by default.

If it is too hard to decide, how about providing a config to let users to choose which to use: written pages only, or written pages plus buffered values?

If we choose 1, we can change to estimate batch size based on avg row size and written row numbers to avoid ignoring too many buffered bytes.

This approach also have obvious caveat since data pages usually do not have same boundary of rows.

I prefer 3 too, this is also the approach taken by parquet-java. And it makes the final row group size smaller than max_row_group_bytes, which is intuitive.

Is there a particular reason for this value? AFAIK some Parquet implementation (is it Parquet Rust? @alamb ) writes a single row group per file by default.

I also feel like the HDFS-related reasons in the Parquet docs are completely outdated (who cares about HDFS?).

I think smaller row groups are still useful when pruning is essential. https://www.firebolt.io/blog/unlocking-faster-iceberg-queries-the-writer-optimizations-you-are-missing is a good read.

Right, but the value is not easy to devise. For example, if you have 10_000 columns, this will make for some very short columns.

In this case, we could set a very large value as default to keep the current behavior. Some engines are smart enough to derive a good threshold (from history or whatever source).

Is there a particular reason for this value? AFAIK some Parquet implementation (is it Parquet Rust? @alamb ) writes a single row group per file by default.

The default row group size in the rust writer is 1M rows (1024*1024) -- NOT bytes

https://docs.rs/parquet/latest/parquet/file/properties/struct.WriterPropertiesBuilder.html#method.set_max_row_group_size

I looked through and didn't find any setting for max row group size in bytes.

I believe at least at some point in the past, the DuckDB Parquet writer wrote a single large row group -- I am not sure if that is the current behavior or not

I think Apache Impala also produces single row group in its Parquet writer. However I think limiting row group size in bytes is still useful in some cases. For example, there is a table property in Iceberg: https://github.com/apache/iceberg/blob/73a26fc1f49e6749656a273b2e4d78eb9e64f19e/docs/docs/configuration.md?plain=1#L46. As iceberg-cpp is depending on the Parquet writer here, it is nice to support this feature.

In many common cases, the compression ratio is close to 3:1. So I used something like total_compressed_bytes + total_compressed_bytes_written + EstimatedBufferedValueBytes / (codec_type != NONE ? 3 : 1) as an empirical value in the past.
`

We cannot accept infinite loop so perhaps we have to set the minimum batch size to 1 in this case?

Unrelated to this PR: perhaps it is also useful to provide an estimation of the current file size to facilitate downstream to implement a rolling file writer.

I think Apache Impala also produces single row group in its Parquet writer. However I think limiting row group size in bytes is still useful in some cases. For example, there is a table property in Iceberg: https://github.com/apache/iceberg/blob/73a26fc1f49e6749656a273b2e4d78eb9e64f19e/docs/docs/configuration.md?plain=1#L46. As iceberg-cpp is depending on the Parquet writer here, it is nice to support this feature.

contents_->EstimatedBufferedValueBytes() may under-estimate the size because it only accounts for the values buffered by current_encoder_, and ignores buffered values in the repetition_levels_rle_, definition_levels_rle_ and current_dict_encoder_. For deep nested types, level values may be significantly larger than the values.

Nice catch, buffered levels size is available from definition_levels_sink_ and repetition_levels_sink_.

We need to check if avg_row_size is 0 or NaN to skip the calculation below. Perhaps we can check this in EstimateCompressedBytesPerRow() to return std::nullopt.

EstimateCompressedBytesPerRow() return calculated value only when the num_rows > 0, so it's never 0.

How do you achieve the step 4 above next loop to 1? This looks a bit complicated to me. How about the logic below.

For each loop iteration:

1. Check the row group threshold (both rows and bytes) and append a new row group if needed.
2. Compute a new batch_size based on different conditions and set its minimum to 1 so we don't get empty batch.
3. Write the batch as before.